Skip to content

fix(mysql): make PITR disk-pressure binlog cleanup functional and prefix-safe#3088

Draft
weicao wants to merge 6 commits into
mainfrom
helios/mysql-pitr-binlog-cleanup
Draft

fix(mysql): make PITR disk-pressure binlog cleanup functional and prefix-safe#3088
weicao wants to merge 6 commits into
mainfrom
helios/mysql-pitr-binlog-cleanup

Conversation

@weicao

@weicao weicao commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Fixes #3087.

Problem

The PITR disk-pressure cleanup path could misclassify binlog state or continue after command failures:

  • replica discovery relied on environment variables that are not available in a DataProtection job;
  • the purge loop could delete beyond the safe uploaded/synchronized prefix;
  • substring membership checks could match the wrong binlog;
  • uploaded-object discovery matched the target Pod name instead of the configured LOG_PREFIX;
  • failures inside the cleanup function were not guaranteed to stop the script.

Changes

  • discover replicas through the primary using SHOW REPLICAS, with the MySQL 5.7 fallback;
  • compute one conservative purge boundary and use PURGE BINARY LOGS TO only for the safe prefix;
  • use exact newline membership checks;
  • match uploaded objects using LOG_PREFIX;
  • enable fail-fast shell behavior while preserving the deliberate no-replica skip boundary.

Validation

  • bash -n on the touched script: PASS
  • helm lint addons/mysql: PASS
  • full chart render: PASS
  • git diff --check: PASS
  • exact-head PR CI is green except the currently running ShellSpec job

Evidence Boundary

These are source, syntax, render, and CI gates. Runtime PITR/disk-pressure behavior remains N=0 and belongs to the test owner.

@codecov-commenter

codecov-commenter commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (b55707f) to head (774786e).

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #3088   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files        147     147           
  Lines      23282   23282           
=====================================
  Misses     23282   23282           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@weicao weicao added the nopick Not auto cherry-pick when PR merged label Jul 8, 2026
wei and others added 5 commits July 13, 2026 20:54
…fix-safe

Replica discovery relied on KB_ITS_*_HOSTNAME env that never existed in
DataProtection job pods on KB 1.x, so the >=80% disk cleanup (default
PURGE_BINLOG=on) silently never purged anything. Discover replicas from
the target primary via SHOW REPLICAS / SHOW SLAVE HOSTS instead.

Also make the master purge prefix-safe: PURGE BINARY LOGS TO is prefix
deletion, so scan stops at the first file that must be kept (not yet
synced+uploaded, or one of the newest 5) and a single PURGE TO that
file is issued - the old per-file loop could delete an unarchived
binlog sitting before a qualifying one and tear the archived sequence.
Membership tests are now exact-match instead of substring grep.

Fixes #3087

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The disk-pressure binlog purge only triggers at >=80% usage, which cannot
be safely reached on a large shared host volume in tests (would need a
~177GB fill). Make the threshold overridable via PURGE_BINLOG_DISK_THRESHOLD
(default 80, production behaviour unchanged) so a test can set it below the
current usage and exercise the real purge path without filling the disk.

Refs #3087

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
get_synced_binlogs matched replica status against a reconstructed
${DP_TARGET_POD_NAME}-bin, which fails when the target env drifts from
the actual binlog-writing pod (e.g. after a switchover) - returning
'No synced binlog files found' even when replicas are synced. Match
against LOG_PREFIX (derived from the real log_bin_basename) instead.
Add DEBUG lines printing DP_TARGET_POD_NAME/LOG_PREFIX/replica hosts and
per-replica resolved current_file, so the cleanup-decision path is
observable in the archive-binlog log.

Refs #3087

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…stderr

get_replica_hosts assumed replicas register report_host, but the semisync
cmpd start command omits --report-host (only the MGR cmpd and
mysql-entrypoint.sh set it). With report_host empty, SHOW REPLICAS leaves the
Host column blank and `awk '{print $2}'` slides onto the Port, returning
"3306" as the replica host (observed: 'Processing replica host: 3306',
'Can't connect to MySQL server on 3306:3306'). Discover connected replicas
from the primary's own Binlog Dump threads instead
(information_schema.PROCESSLIST), which carry the replica ip:port regardless
of report_host; strip the ephemeral :port. Fall back to SHOW REPLICAS / SHOW
SLAVE HOSTS with TAB-split awk so an empty Host stays empty instead of
becoming the Port.

Also move the two DEBUG lines in get_synced_binlogs to stderr: that
function's stdout is captured by `synced_binlogs=$(get_synced_binlogs)`, so
echoing on stdout both hid the lines from the pod log and corrupted the
return value (making the empty result look non-empty and bypassing the
fail-closed 'No synced binlog files found' guard).

Split the `local synced_binlogs=$(...)` declaration from its assignment so
the `$? -ne 0` check reflects get_synced_binlogs' exit code rather than the
always-zero exit of `local`, keeping the fail-closed guard closed.

Refs #3087

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two data-safety defects found in a deep review of the PITR binlog cleanup.

1. The fail-closed guard `synced_binlogs=$(get_synced_binlogs); if [ $? -ne 0
   ]` is unreachable under the actionset-wide `set -e`: a separated
   `var=$(fn)` assignment where fn returns 1 aborts the whole script before
   the check, so a replica-parse failure crash-loops the archiver instead of
   logging and skipping the purge. (Splitting the earlier `local var=$(...)`
   to fix `$?` masking removed the mask that `set -e` was relying on.) Use
   `if ! synced_binlogs=$(get_synced_binlogs) || [ -z ... ]` so the failure
   is caught without tripping set -e.

2. `min_synced_file` was a leaked global, never reset. On a 2nd+ cleanup call
   in the long-lived archiver, if replica discovery returns empty the loop is
   skipped and the stale previous value passes the guard, purging binlogs on
   an unverified synced position (potential data loss). Declare it `local`
   with an empty reset at function entry.

Both verified: bash confirms the separated assignment aborts under set -e
while `if !` does not; helm render + bash -n pass.
@weicao
weicao force-pushed the helios/mysql-pitr-binlog-cleanup branch from 52d78d4 to 7dcbd13 Compare July 13, 2026 12:54
@weicao

weicao commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Absorbed the remaining follow-up from PR #3157 at head 774786e. The existing main-based PR already contained the set-e and stale-state guards; this update additionally matches uploaded binlogs by the live LOG_PREFIX instead of stale DP_TARGET_POD_NAME. bash -n, helm lint, and git diff --check pass. PR #3157 is superseded by this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

nopick Not auto cherry-pick when PR merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mysql: PITR disk-pressure binlog cleanup is a silent no-op (dead KB_ITS env) with a latent purge-gap defect

2 participants